-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Badger: v1 to v3 upgrade #3096
Badger: v1 to v3 upgrade #3096
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3096 +/- ##
==========================================
- Coverage 95.91% 95.91% -0.01%
==========================================
Files 236 236
Lines 10252 10251 -1
==========================================
- Hits 9833 9832 -1
- Misses 348 349 +1
+ Partials 71 70 -1
Continue to review full report at Codecov.
|
If we want to avoid a breaking change, we could deprecate support for badger v1 (no longer maintain it, along with clear warning logs) and create another span storage type badger_v3. Then eventually decommission badger v1 after a few releases. |
How do people migrate data from version to version in badger? Is there any way to migrate? Would it require writing a custom tool? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I don't think we should keep v1 around.
Please edit the change log and describe the breaking changes, such as renaming of metrics.
Thank you everyone for the review. I will work on fixing the build failures and other feedbacks.
Reading through the badger github issues, it seems like they suggest to do backup and restore which seems compatible across different versions. |
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
This PR is ready for review again. Thank you. |
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
This PR is ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of nits, otherwise LGTM.
Signed-off-by: Ashmita Bohara <[email protected]>
@@ -136,7 +140,7 @@ func addFlags(flagSet *flag.FlagSet, nsConfig NamespaceConfig) { | |||
flagSet.Bool( | |||
nsConfig.namespace+suffixTruncate, | |||
nsConfig.Truncate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this field be removed from config struct ?
Signed-off-by: Ashmita Bohara <[email protected]>
Could you add a link to this in the changelog? So that people know where the migration instructions are located. |
I tried the steps and thought to include as part of documentation: jaegertracing/documentation#507 |
Signed-off-by: Ashmita Bohara [email protected]
Which problem is this PR solving?
Relates to #2459 #2987
Short description of the changes
Benchmarks
Badger is well known for high memory usage. We are using very version of badger which makes it almost unusable. I did some benchmark between badger-v1 and badger-v3 using all-in-one image and tracegen.
Badger v1 benchmarks
Started all-in-one with badger-v1 backend from master:
Started tracegen:
Result:
From htop:
Badger v3 benchmarks
Started all-in-one with badger-v3 backend from PR branch:
Started tracegen:
Result:
From htop:
I see good performance gain in terms of all three resources: cpu, memory and disk.
Although this is a breaking change but I think we should move in this direction. We know badger makes breaking changes in every major version release.